Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(forms): type NG_VALUE_ACCESSOR injection token correctly #29723

Closed
wants to merge 1 commit into from

Conversation

Airblader
Copy link
Contributor

@Airblader Airblader commented Apr 5, 2019

fixes #29351

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

NG_VALUE_ACCESSOR is typed ControlValueAccessor. This conflicts with it being used as multi: true when injecting it programmatically through Injector#get as the function is typed to return a single ControlValueAccessor, even though it should be an array of them.

Issue Number: #29351

What is the new behavior?

The injection token is now typed ControlValueAccessor[].

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I have not added any tests for this as it's purely a type information. This cannot be tested in an assertion kind of test, the only possibility would be a nop Injector#get call and making sure the TS compiler doesn't complain. This isn't a functional test, though, and I doubt we generally test that types are correct, so it seems unnecessary.

@Airblader
Copy link
Contributor Author

Regarding the two failed CI jobs:

  1. Yes, technically the public API changes, but see my description above for why I believe this is not an issue here.
  2. The failing lifecycle hook tests seem unrelated to this change(?)

@ngbot ngbot bot added this to the needsTriage milestone Apr 17, 2019
kara
kara previously requested changes May 7, 2019
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

A few things need to happen before this can be merged:

  • Run public_api guard task to update goldens (cannot be merged while this test is red)
    - yarn bazel run //tools/public_api_guard:forms_api.accept
  • Update commit message to add more documentation about the change
  • [Kara] Run through Google3 TAP to see how much of a breaking change this would be

@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: major This PR is targeted for the next major release action: presubmit The PR is in need of a google3 presubmit labels May 7, 2019
@kara
Copy link
Contributor

kara commented May 7, 2019

presubmit

@Airblader
Copy link
Contributor Author

@kara The command you listed fails for me with like below. Nonetheless, I think I should have updated everything accordingly.

$ bazel run //tools/public_api_guard:forms_api.accept
INFO: Options provided by the client:
  Inherited 'common' options: --isatty=1 --terminal_columns=140
INFO: Reading rc options for 'run' from /home/ingo/repos/angular/.bazelrc:
  Inherited 'build' options: --strategy=AngularTemplateCompile=worker --strategy=TypeScriptCompile=standalone --symlink_prefix=dist/ --nowatchfs --incompatible_strict_action_env --experimental_ui --define=compile=legacy
ERROR: Unrecognized option: --incompatible_strict_action_env
error Command failed with exit code 2.

@pullapprove pullapprove bot requested a review from AndrewKushnir May 7, 2020 21:31
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label May 7, 2020
@AndrewKushnir
Copy link
Contributor

Hi @Airblader,

This change will most likely be breaking, so we can see if we can include it into one of the next major releases (v11). Could you please rebase this PR when you get a chance, so that I can run a new presubmit to estimate the effort that would be required to adapt g3 apps code to this change?

Thank you,
Andrew

@Airblader
Copy link
Contributor Author

@AndrewKushnir I've rebased it.

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented May 8, 2020

Thanks for the rebase @Airblader 👍 I've started g3 presubmit (+ global presubmit) to estimate the effort that'd be required to land this change.

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels May 8, 2020
@AndrewKushnir AndrewKushnir removed the request for review from a team May 8, 2020 18:20
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label May 20, 2020
@AndrewKushnir
Copy link
Contributor

@Airblader, thanks for updating the PR, it looks like a couple CI jobs are failing (lint and test). Could you please have a look at the failures and update the code? Thank you.

@Airblader Airblader force-pushed the bug-29351 branch 2 times, most recently from e488146 to a2f7c57 Compare September 10, 2020 07:28
@Airblader
Copy link
Contributor Author

@AndrewKushnir Should be fixed now.

@Airblader
Copy link
Contributor Author

I had that, but the tests failed because they said it has to be like this.

@AndrewKushnir
Copy link
Contributor

@Airblader could you please try to apply the changes to the packages/forms/src/directives/control_value_accessor.ts file and run the following command after that (it should update the goldens/public-api/forms/forms.d.ts file, the .d.ts files are generated by the script):

yarn public-api:update

If there are failing tests after these changes, please let us know, we'll look into that.

Thank you.

NG_VALUE_ACCESSOR is a multi injection token, users can and
should expect more than one ControlValueAccessor to be
available (and this is how it is used in @angular/forms).

This is now reflected in the definition of the injection token
by typing it as an array of ControlValueAccessor. The motivating
reason is that using the programmatic Injector api will now
type Injector#get correspondingly.

fixes angular#29351

BREAKING CHANGES

NG_VALUE_ACCESSOR is now typed as a readonly array rather than
a mutable scalar. It is used as a multi injection token and as
such it should always be expected that more than one accessor
may be returned.
@Airblader
Copy link
Contributor Author

Airblader commented Sep 10, 2020

@AndrewKushnir I updated it and ran the yarn command, which did not change the public-api to the same but instead retained readonly X[].

Edit: Now updating it manually in the public-api and re-running the yarn command to see if it will undo that again. I won't push that but will let you know here what happened.

Edit 2: Yeah, it undoes it and changes it back to readonly X[].

@AndrewKushnir
Copy link
Contributor

Presubmit + Global Presubmit.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Sep 14, 2020
@AndrewKushnir
Copy link
Contributor

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: public-api

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: public-api

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Sep 21, 2020
@AndrewKushnir
Copy link
Contributor

Quick update:

  • tests in Google's codebase went well (including Global run)
  • there were no cases in Google's codebase that required a cleanup due to this change (no targets were broken)
  • there are no migration planned for this change due to low probability of breakages due to this change
  • this PR has all the necessary approvals now and I'm adding it to the merge queue

@Airblader thanks for contributing to Angular! 👍

@AndrewKushnir AndrewKushnir added the action: merge The PR is ready for merge by the caretaker label Sep 21, 2020
@mhevery mhevery closed this in 2b1b718 Sep 21, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: forms breaking changes cla: yes cross-cutting: types target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Injector#get is typed incorrectly
7 participants